This seems like a lot of change.

Have you tried either of these ways?: STI or ignoring the post in the comment model.

STI would have Comment as a child class of Post. Not sure that I like this, but some people like STI.

Treat the Comment as a tree form, but just have a belongs_to :post?
So you just bring back all comments arranged, via:

post.comments.roots
Comment.where(:post_id => post.id).roots

Besides roots, you can call any scope/method that works on Comment

This seems like a lot of change.

Well, the challenging thing for this change was tests. I would say 90% of this PR is setting up the test project + specs. Otherwise we're using almost the same code with a bit of changes. But because I don't want to break any backwards compatibility there was a bit of copy paste.

About STI, I would really like to avoid it at all cost. It has cause me a lot of pain the past.

About your other solution, I'm not sure if it will provide the same benefits of n+1 query avoidance. If I have 100 roots (eg. 100 root comments on a post), it will result in 100 queries for the children of each of the root nodes? Perhaps I'm missing something from the eager loading methods, like root_comment_including_tree

If anyone would like to help me add documentation on how to handle eager loading with multi root has_many, I'm happy to participate in whatever way I can.

To summarize -- There are 300+ lines but the core of the change are just these lines

          # For multiple roots, fetch all descendants at once to avoid N+1 queries
          root_ids = roots.map(&:id)
          klass = roots.first.class
          hierarchy_table = klass._ct.quoted_hierarchy_table_name

          # Get all descendants of all roots including the roots themselves
          # (generations 0 = self, > 0 = descendants)
          descendant_scope = klass.
            joins("INNER JOIN #{hierarchy_table} ON #{hierarchy_table}.descendant_id = #{klass.quoted_table_name}.#{klass.primary_key}").
            where("#{hierarchy_table}.ancestor_id IN (?)", root_ids).
            includes(*assoc_map).
            distinct

          descendant_scope

About STI, I would really like to avoid it at all cost. It has cause me a lot of pain the past.

lol - yes. My teammates no longer listen when I say "lets get rid of the STI in model X." Different people are in different places on that debate. Seems many of the users of ancestry rely upon STI in the darndest places. (sorry, ancestry is more my point of reference - just trying to help a little over here, too.)

I'm not sure if it will provide the same benefits of n+1 query avoidance

I think we agree that the perfect case requires 2 queries: Post and Comment. And the perfect case would be to state it as a single query on Post with the Comment query being driven by an includes(:comments) or closure tree specific scope/method stating that.

Wish it were easier to say to active record: "I've fetched these records because you didn't know how to fetch them for includes(). Please put them into these objects as if it were fetched using includes()

I was thinking:

posts = Post.take(10)
comments = Comment.where(:post_id => posts.map(&:id)).hash_tree

posts.each do |post|
  puts "#{post.id}: #{post.name}"
  print_comments(comments.keys.select { |comment| comment.post_id == post.id})
end

def print_comments(comments_hash, indent: 0)
  comments_hash.each do |comment, children|
    puts " " * indent + comment.text
    print_comments(children, indent: indent + 2) unless children.blank?
  end
end